-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
resolve homework jv-salary-info #1236
base: master
Are you sure you want to change the base?
Conversation
SimpleDateFormat format = new SimpleDateFormat("dd.MM.yyyy"); | ||
Date docDate = null; | ||
Date fromDate = null; | ||
Date toDate = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please review checklist of common mistakes and we can continue with review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to extract some code into separate method to improve readability
int sum = 0; | ||
for (String elementOfData : data) { | ||
String[] elements = elementOfData.split(" "); | ||
if (elements[1].equals(name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is 1? Make a constant with an informative name
LocalDate fromDate = null; | ||
LocalDate toDate = null; | ||
try { | ||
docDate = LocalDate.parse(elements[0], formatter);; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is 0?
for (String elementOfData : data) { | ||
String[] elements = elementOfData.split(" "); | ||
if (elements[1].equals(name)) { | ||
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read common mistakes before sending your solution to review
} | ||
} | ||
if (count == names.length - 1) { | ||
result.append(name).append(" - ").append(sum); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" - " also should be a const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's improve your solution one more time ;)
private static final int WORKING_HOUR = 2; | ||
private static final int INCOME_PER_HOUR = 3; | ||
private static final String SEPARATOR = " - "; | ||
private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong naming for constant
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
StringBuilder result = new StringBuilder("Report for period " | ||
+ dateFrom + SEPARATOR + dateTo + "\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use System.lineSeparator instead of \n
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
StringBuilder result = new StringBuilder("Report for period " | ||
+ dateFrom + SEPARATOR + dateTo + System.lineSeparator()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringBuilder is used to avoid creating new String during the concatenation, so use append() method
fromDate = fromDate.minusDays(1); | ||
toDate = toDate.plusDays(1); | ||
} catch (DateTimeParseException e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad approach, it's better to throw an exception with informative message
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) { | ||
return null; | ||
StringBuilder result = new StringBuilder("Report for period ").append(dateFrom) | ||
.append(SEPARATOR).append(dateTo).append(System.lineSeparator()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.append(SEPARATOR).append(dateTo).append(System.lineSeparator()); | |
.append(SEPARATOR).append(dateTo); |
if (count == names.length - 1) { | ||
result.append(name).append(SEPARATOR).append(earnedMoneyByPerson); | ||
} else { | ||
result.append(name).append(SEPARATOR).append(earnedMoneyByPerson) | ||
.append(System.lineSeparator()); | ||
count++; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove if-else, add lineseparator at the beginning, not the end of the line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
No description provided.